-
Notifications
You must be signed in to change notification settings - Fork 331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support initial aria-describedby on all form fields #1347
Support initial aria-describedby on all form fields #1347
Conversation
One suggestion, is perhaps we rename it Similar to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this generally makes sense, and the tests are really thorough 👏 Thanks @colinrotherham!
There are a couple of minor inconsistencies and some documentation issues that need addressing before I can approve it.
@@ -9,7 +9,10 @@ | |||
|
|||
{#- a record of other elements that we need to associate with the input using | |||
aria-describedby – for example hints or error messages -#} | |||
{% set describedBy = "" %} | |||
{% set describedBy = params.describedBy if params.describedBy else "" %} | |||
{% if params.fieldset.describedBy %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right in thinking there's no difference between passing { fieldset : { describedBy: 'foo' }}
and { describedBy: 'foo' }
? Wondering if we need to support both of those, or if we should just support passing it on the fieldset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also be consistent with the date input and radio components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@36degrees It's because checkboxes are special, unlike radios.
Checkboxes with single option (and hint) set 'aria-describedby' on input
So the describedBy
attribute can be without a fieldset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, in which case I think this makes sense, thanks for talking me through it.
I can't see a test that covers this functionality (setting aria-describedby on a single checkbox without a fieldset) – could we add one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! Thanks.
I've extended the existing single checkbox tests and made two new ones.
You can see the new bit pushed here: https://github.com/alphagov/govuk-frontend/compare/8e23ce86235c93a77d54b7ccccb0edad923ef7e5..09a9700814d433925b52a9f6d25848093cacdd61
Thanks @36degrees, I'll fix the params:
- name: describedBy
type: string
required: false
description: Text or element id to add to the `aria-describedby` attribute to provide description of the group of fields for screenreader users. I'll remove the "Text or" bit from all mine too, so it's just the element id. |
Hah, of course it came from us 🤦♂ |
7f28b36
to
8e23ce8
Compare
@36degrees Thanks for the thorough review 👍 I've left two of the conversations open regarding the single checkbox without a fieldset. You might not want to support customising |
8e23ce8
to
09a9700
Compare
Two new tests added to exercise the single checkbox variations:
The new tests show how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @colinrotherham.
This'll need another review from the team and then we can get this merged.
As you've probably noticed, we're working up to a 3.x release at the minute, which may take a little while. Is this functionality you need with any urgency? We can look at doing a 2.x release if it's required.
@36degrees Perfect, thank you. No immediate urgency. Would be a great one to have alongside 3.x 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of work, thanks a lot @colinrotherham
We've noticed the
describedBy
param used internally can't be configured.This pull request supports an initial
aria-describedby
value on all form fields, populated before the optional hint and error message IDs are appended.For form inputs:
Or for those wrapped by fieldsets:
Useful when fields are described by errors or hints on parent fieldsets such as:
When moving between these nested checkboxes, we can now use
params.fieldset.describedBy
to repeat the error message "Select at least one reason" to screen readers as a usability improvement.